Skip to content

task_list_api -Sunitha -Sphinx#34

Open
nelasunitha wants to merge 17 commits intoAda-C22:mainfrom
nelasunitha:main
Open

task_list_api -Sunitha -Sphinx#34
nelasunitha wants to merge 17 commits intoAda-C22:mainfrom
nelasunitha:main

Conversation

@nelasunitha
Copy link

Please review.

Copy link

@yangashley yangashley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work on task-list-api!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before adding files to your commit history, review what files you have staged by running git status. Changes like the ones here that are not meaningful should not be a part of your pull request because that's essentially asking to merge in unnecessary changes to the codebase which can make the codebase messy.

from ..db import db
from datetime import datetime

goals_bp = Blueprint("goals_bp", __name__, url_prefix="/goals")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Learn and in the livecode for Flasky, we name blueprints for each model bp. In goal_routes.py I can deduce that bp is the blueprint for Goal given the name of the file.

If we have multiple route files that each have a blueprint named bp, we can account for name collisions when importing them into app/__init__.py by using aliases:

from .routes.task_routes import bp as tasks_bp
from .routes.goal_routes import bp as goals_bp

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless these changes were required, I'd revert unnecessary changes so they don't get committed and reviewed when you open a PR.


class Goal(db.Model):
id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
title: Mapped[str]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider whether the this column for Goal should be nullable. It feels odd to allow someone to create a goal that gets saved to the DB without a title.

Comment on lines +10 to +15
def to_dict(self):
return dict(id=self.id, title=self.title)

@classmethod
def from_dict(cls, goal_data):
return cls(title=goal_data["title"])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@goals_bp.get("/<goal_id>/tasks")
def get_tasks_by_goal(goal_id):
goal = validate_goal(goal_id)
goal_dict = goal.to_dict()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While we are creating a dictionary that represents a goal, it might be more descriptive to name this variable response since we're creating a dictionary that we want to send back as a response to the client.

from datetime import datetime
from app.models.task import Task

tasks_bp = Blueprint("tasks_bp", __name__, url_prefix="/tasks")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above in goal_routes.py, could be named just bp

Comment on lines +13 to +24
request_body = request.get_json()

try:
new_task = Task.from_dict(request_body)

except KeyError:
response = {"details": "Invalid data"}
abort(make_response(response, 400))


db.session.add(new_task)
db.session.commit()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above in goal_routes.py, would prefer this logic to be in a helper function so you're not duplicating very similar code across different files.

Comment on lines +58 to +61
task = validate_task(task_id)


return {"task": task.to_dict()}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +78 to +83
def mark_task_complete(task_id):
task = validate_task(task_id)
task.completed_at = datetime.now()
db.session.commit()

return {"task": task.to_dict()}, 200

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the sake of time, I went to your repo and found your other branch to look at your Slack implementation. Note: I cannot leave comments on a codebase. That's why we have you open PRs for your projects. In the future, please make sure all of your solution is in your PR.

I'm copy and pasting your helper function here and leaving my review comments as comments in your code

def send_slack_message(task_title):
    # constants should be named using all capital letters
    token = os.environ.get("SLACK_BOT_TOKEN")
    if not token:
        raise ValueError("Slack Bot Token is not set in environment variables.")

    headers = {"Authorization": f"Bearer {token}", "Content-Type": "application/json"}
    payload = {"channel": SLACK_CHANNEL, "text": f"Beautiful task {task_title}"}

    response = requests.post(SLACK_API_URL, headers=headers, json=payload)
    if response.status_code != 200:
        print("Failed to send Slack message:", response.json())
    else:
        print("Slack message sent successfully.")

@nelasunitha nelasunitha changed the title ttask_list_api -Sunitha -Sphinx task_list_api -Sunitha -Sphinx Dec 6, 2024
nelasunitha and others added 6 commits December 6, 2024 11:56
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Co-authored-by: Ashley Yang <ashley.minghui@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants